Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add feature flag to restrict access to the inventory report button to sysadmins. (PP-1329) #115

Merged
merged 5 commits into from
Jun 12, 2024
Merged

Add feature flag to restrict access to the inventory report button to sysadmins. (PP-1329) #115

merged 5 commits into from
Jun 12, 2024

Conversation

tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Jun 10, 2024 •
edited by jira bot
Loading

Description

  • Adds the reportsOnlyForSysadmins feature flag, which, when set to true (the default) prevents display of the inventory requests download button for users without the system admin role.
  • Adds feature flags into the context provider.

Motivation and Context

  • Need to prevent regular users from using the inventory report feature until it has been sufficiently tested.
  • Since feature flags are set only once per app instantiation, they don't really need to be tracked as state. Removing them from state reduces complexity of both understanding the app and migrating state to new frameworks. Currently they exist as both state and context, but the goal is to eventually remove them from state.

[Jira PP-1329]

How Has This Been Tested?

  • Manually tested in local dev environment.
  • Tests passing locally.
  • CI tests passing.

Checklist:

  • N/A - I have updated the documentation accordingly.
  • All new and existing tests passed.

@tdilauro tdilauro requested a review from ray-lee June 10, 2024 20:54
ray-lee
ray-lee approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@ray-lee ray-lee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a typo in a comment. Otherwise looks good!

src/components/LibraryStats.tsx Outdated
@@ -48,7 +51,13 @@ const inventoryKeyToLabelMap = {

/** Displays statistics about patrons, licenses, and collections from the server,
for a single library or all libraries the admin has access to. */
const LibraryStats = (props: LibraryStatsProps) => {
// *** To use the legacy context here, we need to create a `contextProps` property on this function object:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean "contextTypes" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ray-lee! I was trying to say a couple things at the same time and got them muddled. I have clarified, fixed a copypasta, and fixed my previous comment that I copied from.

@tdilauro tdilauro force-pushed the feature/flag-inv-reports-sysadmin-only branch from a307562 to 165297d Compare June 12, 2024 12:59
@tdilauro tdilauro merged commit 24dde11 into main Jun 12, 2024
1 check passed
@tdilauro tdilauro deleted the feature/flag-inv-reports-sysadmin-only branch June 12, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ray-lee ray-lee ray-lee approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tdilauro @ray-lee